Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: anoncreds w3c migration #1744

Merged

Conversation

auer-martin
Copy link
Contributor

No description provided.

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lot of work @2mau!!! I've left quite some comments. Most of them are small housekeeping fixes, some require some more substantial refacotrs. I'm fine with possibly moving some improvements over to after this is merged and released, but especially the API surface can sometimes use some clenaup I think. The logic itself makes sense though and I think you made the right abstractions, nice job 👍

@TimoGlastra
Copy link
Contributor

@2mau #1754 has just been merged. The tests were flaky and almost always failing. There are some changes however to e2e tests vs unit tests.

Could you make sure that all the tests you edited / added are following the new pattern:

  • e2e test: uses indy / cheqd / postgres / http / ws
  • unit test: other tests, can run without internet and external services

If a test is a e2e test it should end with .e2e.test.ts

An easy way to check is running the unit tests (yarn test:unit) without any of the docker-compose services running and any tests that fail because the service is not running should use .e2e.test.ts. Other way around is a bit harder, and is a bit more manual. Not sure how much of tests you wrote are using in memory anoncreds registry (preffered) vs indy/cheqd registry (for a few e2e tests)

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of good updates! I left some additional comments. Also some comments I left on previous review seemed unresolved but marked as resolved

Comment on lines -464 to -466
if (!isUnqualifiedSchemaId(schemaId)) {
throw new CredoError(`${schemaId} is not a valid legacy indy schema id`)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This service should not support qualified ids. It's a legacy service (LegacyIndyProofFormatService), and was from before qualifieid identifiers existed.

Comment on lines 213 to 243
const missingBindingMethod =
binding_required && !binding_method?.anoncreds_link_secret && !binding_method?.didcomm_signed_attachment

const invalidDataModelVersions =
!data_model_versions_supported ||
data_model_versions_supported.length === 0 ||
data_model_versions_supported.some((v) => v !== '1.1' && v !== '2.0')

const invalidLinkSecretBindingMethod =
binding_method?.anoncreds_link_secret &&
(!binding_method.anoncreds_link_secret.cred_def_id ||
!binding_method.anoncreds_link_secret.key_correctness_proof ||
!binding_method.anoncreds_link_secret.nonce)

const invalidDidCommSignedAttachmentBindingMethod =
binding_method?.didcomm_signed_attachment &&
(!binding_method.didcomm_signed_attachment.algs_supported ||
!binding_method.didcomm_signed_attachment.did_methods_supported ||
!binding_method.didcomm_signed_attachment.nonce)

if (
missingBindingMethod ||
invalidDataModelVersions ||
invalidLinkSecretBindingMethod ||
invalidDidCommSignedAttachmentBindingMethod
) {
throw new ProblemReportError('Invalid credential offer', {
problemCode: CredentialProblemReportReason.IssuanceAbandoned,
})
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not resolved?

@auer-martin auer-martin marked this pull request as ready for review February 19, 2024 14:45
@karimStekelenburg karimStekelenburg dismissed TimoGlastra’s stale review February 23, 2024 13:43

Changes are addressed. Any open questions can be addressed in a follow-up PR

@karimStekelenburg karimStekelenburg merged commit d7c2bbb into openwallet-foundation:main Feb 23, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Always store qualified identifiers for AnonCreds objects, still supporting legacy indy identifiers in exchange
3 participants